Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add response_mode=query support for OpenID Connect #297

Merged
merged 6 commits into from
Aug 12, 2019

Conversation

ryan-w
Copy link
Contributor

@ryan-w ryan-w commented Jul 3, 2019

This is a backport from AspNetCore.

@dnfclas
Copy link

dnfclas commented Jul 3, 2019

CLA assistant check
All CLA requirements met.

@Tratcher Tratcher self-requested a review July 3, 2019 17:28
@Tratcher Tratcher self-assigned this Jul 3, 2019
@Tratcher Tratcher added this to the 4.0.2 milestone Jul 3, 2019
@Tratcher
Copy link
Member

Tratcher commented Jul 3, 2019

FYI

  • I know we don't have good OIDC test coverage. I'll do some manual verification as part of the review.
  • We don't have our next release scheduled. I'll review and likely merge this, but we don't know when it will ship.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 3, 2019

@Tratcher Thank you. Once it's merged, I can work from the dev feed until it's released.

@Tratcher
Copy link
Member

Tratcher commented Jul 3, 2019

This feature is incomplete. What provider and configuration did you use to test it?

It has the check that prohibits IdTokens in the query string, but later it has a comment and check that says codes can only be provided with IdTokens. There's also no automatic code redemption (heince the need for the IdToken), only the AuthorizationCodeReceived notification the caller needs to implement. Adding automatic code redemption is a larger feature but I don't think this PR is useful without it.

// code is only accepted with id_token, in this version, hence check for code is inside this if
// OpenIdConnect protocol allows a Code to be received without the id_token
if (string.IsNullOrWhiteSpace(openIdConnectMessage.IdToken))
{
_logger.WriteWarning("The id_token is missing.");
return null;
}

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 3, 2019

@Tratcher Good point. After building and referencing the locally built Nuget packages I receive error messages like this.

Error CS0012 The type 'IDataProtectionProvider' is defined in an assembly that is not referenced. You must add a reference to assembly 'Microsoft.Owin.Security, Version=2.1.0.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35'.

It seems that other assemblies are looking for the signed version of this reference and assembly redirecting does not work. I have so far not been able to work past this to test with my local build. Can you point me in the right direction to get this running locally?

@Tratcher
Copy link
Member

Tratcher commented Jul 3, 2019

No need for packages, you can use this project https://github.com/aspnet/AspNetKatana/blob/dev/tests/Katana.Sandbox.WebServer/Startup.cs to run and debug code in the repo. Feel free to comment out unrelated parts like the social providers.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 9, 2019

I just pushed an update to include the automatic code redemption.

@RupW
Copy link

RupW commented Jul 9, 2019

Brilliant, thanks! One issue: you're still calling Options.ProtocolValidator.ValidateAuthenticationResponse even when openIdConnectMessage is now a token response, which gives me a validation failure.

IDX21335: 'refresh_token' cannot be present in a response message received from Authorization Endpoint.

But otherwise works against django-oidc-provider, which ignores response_mode (and never POSTs) and only looks viable with response_type=code.

Coincidentally I've spent the day trying to hack Owin 3.1.0 OpenIdConnect into working with django-oidc-provider, but I didn't know what the problems were when I set out, and your version is /much/ neater. (And I hit different validator problems, since the logic was different back then.) I did copy the backchannel request pattern from the Google auth provider which passes the HttpClient in to the handler's constructor rather than putting it in Options, but I like Options better.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 9, 2019

@RupW Thanks. I'll take a look at that validation issue.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 9, 2019

I just pushed a fix for the validation issue.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 11, 2019

@Tratcher This should be good to go now. Please let me know if you see any issues.

@Tratcher
Copy link
Member

Understood, I should get back to it next week.

@Tratcher
Copy link
Member

Sorry, something got moved ahead of this, it may be another week or two.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 19, 2019

The identity provider for some applications that I manage has recently changed (outside of my control) and does not support the form post method. Users currently are not able to sign-in to the applications until I get this update in place. Is there a way to get around the issue of needing signed binaries so I can start implementing these changes right away?

@Tratcher
Copy link
Member

As a short term measure you can change the nuspec and csproj to point to stable dependencies:
https://github.com/aspnet/AspNetKatana/blob/dev/src/Microsoft.Owin.Security.OpenIdConnect/Microsoft.Owin.Security.OpenIdConnect.nuspec

<ProjectReference Include="..\Microsoft.Owin.Security\Microsoft.Owin.Security.csproj">

That should allow you to build and run just this package.

@ryan-w
Copy link
Contributor Author

ryan-w commented Jul 19, 2019

Ok, thanks. I will give that a try.

@ryan-w
Copy link
Contributor Author

ryan-w commented Aug 1, 2019

@Tratcher I’m still having issues with the references. Can you work this in this week?

@ryan-w
Copy link
Contributor Author

ryan-w commented Aug 8, 2019

Hey Chris, the last commit should address all the items in the change request. Please let me know if you have any additional feedback so we can keep this moving forward. Thanks!

Copy link
Member

@Tratcher Tratcher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better.

Why is the nonce handled differently from Core?

@Tratcher Tratcher merged commit 569f1c8 into aspnet:dev Aug 12, 2019
@Tratcher
Copy link
Member

Thanks for working through that with us.

@Tratcher
Copy link
Member

FYI: Packages should be updated on myget in about an hour (build 1495).

@ryan-w
Copy link
Contributor Author

ryan-w commented Aug 12, 2019

You're welcome. Thanks for your help.

@Hobray
Copy link

Hobray commented Nov 6, 2019

@Tratcher Any idea when the pull @ryan-w made will be in preview on Nuget? I have a scenario where the code flow auth model is the only option I have and the built in code exchange/redemption is super helpful.

@Tratcher
Copy link
Member

Tratcher commented Nov 6, 2019

@Hobray it's scheduled for this month. In the meantime you can get it on Dev myget feed if you want to test it. https://github.com/aspnet/AspNetKatana#nightly-build-feeds

@Tratcher
Copy link
Member

Tratcher commented Nov 6, 2019

Clarification, we're not planning a nuget.org preview, only myget.org. We plan to release the final 4.1.0 build this month.

@Hobray
Copy link

Hobray commented Nov 6, 2019

@Tratcher Perfect. I'm already using it in a slightly more unconventional way but would clearly prefer to align with an actual release. Knowing that there will be a final build this month is excellent. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants